Make cyclic values safe to format and clone - #623
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens WFL’s Value formatting (Display/Debug) and deep_clone against cyclic container graphs, preventing stack overflows and making cloning preserve cycles/shared identity while isolating clones from sources—supporting reliability/security work tracked in #610.
Changes:
- Add active-path cycle detection and a bounded depth limit to
ValueDisplay/Debug, plustry_borrow()-based formatting to avoidRefCellborrow panics for lists/objects. - Rework
Value::deep_cloneto memoize mutable container placeholders before descending, preserving cycles and aliasing within the cloned graph. - Add Rust-level regression tests for cyclic formatting/cloning and an interpreter-level test for displaying a self-referential list; document behavior in changelog and Dev Diary.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/interpreter/value.rs |
Implements cycle-safe + depth-bounded formatting state and memoized deep-clone of container graphs. |
tests/value_cycle_safety.rs |
Adds Rust regression tests for self-cycles, mutual list/object cycles, shared identity preservation, and max-depth formatting. |
src/interpreter/tests.rs |
Adds interpreter-level regression ensuring display of a self-referential list terminates safely. |
Dev diary/2026-07-16-cycle-safe-values.md |
Documents the motivation, behavior changes, and test coverage for cycle-safe values. |
CHANGELOG.md |
Notes the security/reliability behavior change for cyclic values in formatting and cloning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn deep_clone_container_instance( | ||
| instance: &Rc<RefCell<ContainerInstanceValue>>, | ||
| memo: &mut DeepCloneMemo, | ||
| ) -> Rc<RefCell<ContainerInstanceValue>> { | ||
| let source_id = Rc::as_ptr(instance); | ||
| if let Some(cloned) = memo.container_instances.get(&source_id) { | ||
| return Rc::clone(cloned); | ||
| } | ||
|
|
||
| let cloned = Rc::new(RefCell::new(ContainerInstanceValue { | ||
| container_type: String::new(), | ||
| properties: HashMap::new(), | ||
| parent: None, | ||
| line: 0, | ||
| column: 0, | ||
| })); | ||
| memo.container_instances | ||
| .insert(source_id, Rc::clone(&cloned)); | ||
|
|
||
| let source = instance.borrow(); | ||
| let cloned_properties = source | ||
| .properties | ||
| .iter() | ||
| .map(|(key, value)| (key.clone(), value.deep_clone_with_memo(memo))) | ||
| .collect(); | ||
| let cloned_parent = source | ||
| .parent | ||
| .as_ref() | ||
| .map(|parent| Self::deep_clone_container_instance(parent, memo)); | ||
|
|
||
| *cloned.borrow_mut() = ContainerInstanceValue { | ||
| container_type: source.container_type.clone(), | ||
| properties: cloned_properties, | ||
| parent: cloned_parent, | ||
| line: source.line, | ||
| column: source.column, | ||
| }; | ||
|
|
||
| cloned | ||
| } |
| f, | ||
| "Function({})", | ||
| func.name.as_ref().unwrap_or(&"anonymous".to_string()) | ||
| func.name.as_deref().unwrap_or("anonymous") | ||
| ) | ||
| } |
| impl fmt::Display for Value { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| self.fmt_display_with_state(f, &mut ValueFormatState::default(), 0) | ||
| } |
| Value::ContainerInstance(instance) => { | ||
| let inst = instance.borrow(); | ||
| let cloned_properties = inst | ||
| .properties | ||
| .iter() | ||
| .map(|(k, v)| (k.clone(), v.deep_clone())) | ||
| .collect::<HashMap<_, _>>(); | ||
| let cloned_parent = inst.parent.as_ref().map(|p| { | ||
| // Clone the parent reference, not deep clone (to avoid infinite recursion) | ||
| Rc::clone(p) | ||
| }); | ||
| Value::ContainerInstance(Rc::new(RefCell::new(ContainerInstanceValue { | ||
| container_type: inst.container_type.clone(), | ||
| properties: cloned_properties, | ||
| parent: cloned_parent, | ||
| line: inst.line, | ||
| column: inst.column, | ||
| }))) | ||
| Value::ContainerInstance(Self::deep_clone_container_instance(instance, memo)) | ||
| } |
|
Superseded by #632, which preserves this security fix in the consolidated Rust-source hardening PR. The combined head is mergeable and all required CI checks are green. |
Summary
Value::DisplayandDebugdetect active-path list/object cyclesRefCellborrow panics during diagnosticsdeep_clonememoize mutable containers before descendingSecurity impact
Valid WFL can insert a list into itself. Formatting or deeply cloning such a graph previously recursed until native stack exhaustion, which aborts the process rather than producing a catchable WFL error. Cycles now terminate deterministically as
<cycle>, deep acyclic formatting stops at a bounded depth, and graph cloning reuses memoized placeholders.Validation
git diff --checkProduction readiness